Skip to content

Stop a blocking wait helper from shadowing the pumping ones - #8725

Merged
austinywang merged 1 commit into
manaflow-ai:mainfrom
ejc3:fix/blocking-wait-shadowing
Aug 4, 2026
Merged

austinywang merged 1 commit into
manaflow-ai:mainfrom
ejc3:fix/blocking-wait-shadowing

Conversation

@ejc3

@ejc3 ejc3 commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Three tests fail on main because adding a timeout: argument to a wait silently changes which helper
runs. Two are in the sidebar git probe suites and one is in TabManagerWorkspaceOwnershipTests, and
the product is correct in all three cases.

Why the wait cannot succeed

TabManagerUnitTests.swift and WorkspacePullRequestSidebarTests.swift each define a file-private
waitForCondition that polls by hopping through DispatchQueue.main under XCTWaiter, so the main
queue keeps draining while a test waits. The test target also has a module-scope
waitForCondition(timeout:pollInterval:_:) in CLICodexHookTimeoutRegressionTestSupport.swift that
polls with Thread.sleep and runs no run loop at all.

Swift prefers the overload that fills in fewer defaulted parameters. waitForCondition(timeout: 12.0) { … } leaves only pollInterval defaulted on the module-scope one, but would leave pollInterval,
file and line defaulted on the file-private one, so it binds to the blocking version. A call with
no timeout: argument cannot bind to the module-scope one at all and gets the pumping version. So the
waits that pass a timeout block the main thread for their entire budget, and every neighbouring wait in
the same file does not.

That is fatal for these tests, because they wait on main-actor work. The two probe tests fail on their
very first assertion, before reaching what they mean to check. An initial sidebar git probe is
registered synchronously at schedule time — the state map is written when the ladder task is installed,
and the accessor unions both maps, so a probe reports as active before it has run a single attempt.
Clearing it needs the ladder task's first hop, the detached snapshot, and its await MainActor.run
apply. Twelve seconds of Thread.sleep on the main thread denies all three, so the set cannot drain
and the condition can never become true. The logs show a workspace.gitProbe.schedule … reason=initial
line, then 16.5 seconds of silence with no apply, and the crash reporter logging an ANR partway
through.

None of the three failures carries the "Timed out waiting for condition" message that both
file-private helpers emit on timeout, which is independent confirmation that the blocking overload ran
in each case.

The product was right in all three cases

For the remote-split test, the panel id in the failure is the workspace's original local panel, not the
split. The split panel has no gitProbe.schedule line at all, so the skip guard did fire, and the two
assertions that check the panel is remote both passed. ProbeSchedulingTests.swift in CmuxSidebarGit
pins the same behavior deterministically with a fake host and a manual clock, and it passes.

For the defaults test, a non-repository directory does not leak a probe: the snapshot resolves to "not
found", the refresh stops on the first attempt, and both map entries are removed.
testTrackedWorkspaceGitMetadataPollCandidatesExcludeDirectoriesWithoutResolvedGitMetadata waits on
the same accessor for a plain temp directory and passes in 0.077 seconds, using the pumping helper.

The title test differs from the other two in one way worth noting: its earlier assertions pass and it
fails at the behavior it is named after, rather than before reaching it. Delivery there is genuinely
asynchronous: the .ghosttyDidSetTitle observer is registered
with queue: .main, so it runs as a queued main-queue operation rather than inline with the post, and
the apply is deferred again by the panel title coalescer's default 1/30s delay. A second of
Thread.sleep on the main thread starves both hops.

The change

Rename the blocking helper to waitForConditionBlocking and document what it is for. The three calls
then resolve to their file-private helpers again with the same budgets and poll intervals — no
assertion, timeout, or interval is touched. Its three existing callers wait on a socket accumulator
filled off the main thread, which is what the blocking form is appropriate for, so they keep it.

Renaming rather than shadow-proofing the call sites is what makes this stay fixed: a future
waitForCondition(timeout:) written in a file without a pumping helper now fails to compile instead of
quietly blocking the main thread.

Test plan

Measured on 4253cc2884, one GUI test host at a time, same checkout and DerivedData for both arms:

xcodebuild test -scheme cmux-unit -configuration Debug -destination 'platform=macOS' \
  -only-testing:cmuxTests/WorkspacePullRequestSidebarTests \
  -only-testing:cmuxTests/TabManagerPullRequestProbeTests \
  -only-testing:cmuxTests/TabManagerWorkspaceOwnershipTests \
  -only-testing:cmuxTests/CLICodexHookTimeoutRegressionTests
arm XCTest failures across a 36-test run distinct tests red
main unchanged 15 of 36 9
this branch 10 of 36 6

The three tests that flip are testRemoteSplitSkipsInitialGitMetadataProbe,
testUnrelatedDefaultsChangeDoesNotRestartGitMetadataRefreshes and
testFocusedPanelTitleRefreshesAutoWorkspaceTitleInSplitWorkspace. CLICodexHookTimeoutRegressionTests,
which owns the renamed helper, reports Test run with 8 tests in 1 suite passed in both arms, so its
three callers are unaffected. No host restarts in either arm.

Pull-request CI on this repo runs review bots and security scanners, not the test suite, so the arms
above are the only test evidence this carries.

The six tests still red on this branch fail for reasons unrelated to waits: two git-index fixtures that
describe a state git cannot produce, a scoped socket report sent to a manager AppDelegate cannot
resolve, an async test using the pumping helper from a main-actor job, a stub that watches a subprocess
the product no longer spawns, and a product bug in TabManager.closeWorkspace.

Summary by CodeRabbit

  • Tests
    • Improved automated test synchronization by using a blocking condition helper with clearer timeout/polling behavior.
    • Updated regression test steps to wait for expected asynchronous command snapshots at each stage.
    • Expanded inline documentation for the blocking wait helper to improve readability and reduce flaky timing issues.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d10a1c1e-f791-4e1f-a0db-6e54d9a231a2

📥 Commits

Reviewing files that changed from the base of the PR and between 29e33cd and c5bd939.

📒 Files selected for processing (2)
  • cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift
  • cmuxTests/CLICodexHookTimeoutRegressionTests.swift

📝 Walkthrough

Walkthrough

The blocking condition polling helper is renamed to clarify its behavior, with expanded documentation. Three synchronization points in the Codex timeout regression test now call the renamed helper.

Changes

Codex timeout regression test synchronization

Layer / File(s) Summary
Rename and update blocking wait usage
cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift, cmuxTests/CLICodexHookTimeoutRegressionTests.swift
The polling helper is renamed and documented, and the regression test updates three condition waits to use it.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the main change: renaming the blocking helper to avoid shadowing the pumping wait helpers.
Description check ✅ Passed The description covers the summary, rationale, and testing in detail; only non-critical template sections like demo video, review trigger, and checklist are missing.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed Diff only renames a test helper and updates test calls; no production Swift isolation changes or new MainActor/Sendable issues were introduced.
Cmux Swift Blocking Runtime ✅ Passed Only cmuxTests changed; the renamed helper remains test-only scaffolding, and no production Swift blocking/timing sync was introduced or expanded.
Cmux Browser Automation Off-Main ✅ Passed Diff only renames a test wait helper and updates test calls; no browser.* routing or main-thread automation code changed.
Cmux Expensive Synchronous Load ✅ Passed Diff only renames a test polling helper and updates three test call sites; no agent-history load or main-actor synchronous parsing was added or moved.
Cmux Cache Substitution Correctness ✅ Passed Diff only renames a blocking wait helper in tests; it does not replace any authoritative read with cached/opportunistic data in a persistence, history, undo, or snapshot path.
Cmux No Hacky Sleeps ✅ Passed PASS: The PR only renames a Swift test helper and updates call sites; no new fixed sleeps or delays were introduced, and the rule is out of scope for Swift/test-only code.
Cmux Algorithmic Complexity ✅ Passed Only test-support/test files changed; the helper rename doesn’t add or worsen scalable production-path scans.
Cmux Swift Concurrency ✅ Passed Diff only renames a test helper and updates three test call sites; no new DispatchQueue/Combine/Task pattern is introduced in cmux-owned production code.
Cmux Swift @Concurrent ✅ Passed No async/@Concurrent changes were introduced; the PR only renames a synchronous blocking helper and updates sync test call sites.
Cmux Swift Package Boundaries ✅ Passed Diff only touches cmuxTests files; the changed helper and calls are test fixtures, which the boundary rules explicitly allow.
Cmux Swiftpm Lockfiles ✅ Passed Diff only touches two Swift test files; no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project lockfile changes are present.
Cmux Swift Logging ✅ Passed Diff only renames a test helper and updates calls; no print/debugPrint/dump/NSLog/Logger changes or sensitive logging were added.
Cmux User-Facing Error Privacy ✅ Passed Commit only renames a test helper and updates test call sites/docs; no production user-facing errors or sensitive upstream details are added.
Cmux Full Internationalization ✅ Passed Only test/support code changed; no production user-facing text, locale assets, or web/i18n content was added or modified.
Cmux Swiftui State Layout ✅ Passed Diff only renames a test helper and updates test calls; no SwiftUI state/layout patterns are introduced.
Cmux Architecture Rethink ✅ Passed PASS: The diff is test-only: it renames a blocking wait helper in cmuxTests and updates three socket-accumulator waits; no product lifecycle or ownership path changes.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed Diff only renames a test helper and updates test calls; no user-visible window, controller, or cmuxAuxiliaryWindowIdentifiers changes.
Cmux Source Artifacts ✅ Passed Both changed paths are intentional Swift source/test files; no logs, temp dirs, DerivedData, caches, or other artifact paths appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed PR diff against origin/main only changes cmuxTests files; no production Sources/ file adds a debug/test seam.
Cmux No Ambient Global State ✅ Passed PASS: This is test-only support code; the patch only renames an existing file-scope helper and adds docs, with no new global var, singleton, or static namespace.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes three flaky tests caused by Swift overload resolution silently binding waitForCondition(timeout:) to the module-scope blocking helper instead of the per-file XCTWaiter-pumping helpers. The fix renames the blocking helper to waitForConditionBlocking and adds a doc comment explaining the restriction, so any future call in a file without a pumping helper fails to compile rather than silently starving the main thread.

  • CLICodexHookTimeoutRegressionTestSupport.swift: waitForCondition renamed to waitForConditionBlocking with a doc comment that documents the Thread.sleep behavior and explicitly prohibits main-thread use.
  • CLICodexHookTimeoutRegressionTests.swift: three call sites updated to use the new name; all three wait on commands.snapshot(), a socket accumulator filled off the main thread, which is the correct use case for the blocking form.

Confidence Score: 5/5

Safe to merge — all changes are confined to test files with no production code touched.

The change is a targeted rename in two test-support files. The three updated call sites all wait on a socket accumulator filled by background threads, which is the explicitly documented correct use of the blocking form. The rename itself becomes a compile-time guard against future misuse: any new waitForCondition(timeout:) in a file without a pumping helper will fail to compile rather than silently stalling the main thread. No production logic is modified.

Files Needing Attention: No files require special attention.

Important Files Changed

Filename Overview
cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift Blocking wait helper renamed from waitForCondition to waitForConditionBlocking; doc comment added to explain Thread.sleep behavior and prohibit main-thread use. No logic changes.
cmuxTests/CLICodexHookTimeoutRegressionTests.swift Three call sites updated from waitForCondition to waitForConditionBlocking; all three correctly wait on a socket accumulator filled by background threads, so the blocking form is appropriate.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Before
        A[waitForCondition timeout 2.0] --> B{Swift overload resolution}
        B -->|fewer defaults needed| C[module-scope waitForCondition Thread.sleep blocks main]
        B -.->|more defaults needed| D[file-private waitForCondition XCTWaiter pumps main queue]
        C --> E[Main thread starved - condition never true]
    end
    subgraph After
        F[waitForConditionBlocking timeout 2.0] --> G[module-scope waitForConditionBlocking Thread.sleep explicit]
        H[waitForCondition timeout 2.0] --> I[file-private waitForCondition XCTWaiter pumps main queue]
        J[waitForCondition timeout 2.0 in file without pumping helper] --> K[Compile error - no matching overload]
        G --> L[Socket/file conditions filled off main thread]
        I --> M[Main-actor work can complete]
    end
Loading

Reviews (3): Last reviewed commit: "cmuxTests: stop a blocking wait helper f..." | Re-trigger Greptile

@ejc3
ejc3 force-pushed the fix/blocking-wait-shadowing branch from 29e33cd to c5bd939 Compare July 23, 2026 22:32
Two test files define a file-private `waitForCondition` that polls by hopping
through DispatchQueue.main under XCTWaiter, so the main queue keeps draining while
a test waits. The test target also has a module-scope one that polls with
Thread.sleep and runs no run loop at all.

Swift prefers the overload that fills in fewer defaulted parameters, so
`waitForCondition(timeout: X) { ... }` binds to the module-scope blocking helper --
it only defaults pollInterval, where the file-private one would also default file
and line. A call with no timeout: argument cannot bind to it and gets the pumping
helper. So adding a timeout silently changed which helper ran, and blocked the main
thread for the whole budget.

That is fatal for anything waiting on main-actor work. Three call sites pass an
explicit timeout and all three are red on main:

- testRemoteSplitSkipsInitialGitMetadataProbe and
  testUnrelatedDefaultsChangeDoesNotRestartGitMetadataRefreshes wait for the
  initial sidebar git probe to drain. That probe is registered synchronously at
  schedule time and only clears once the ladder task, the snapshot, and its
  MainActor.run apply get main-actor turns, so a blocking wait denies the very work
  it waits for. Both fail on their first assertion, before reaching what they mean
  to check, with the run wedged long enough that the crash reporter logged an ANR.
  The product is correct in both cases.
- testFocusedPanelTitleRefreshesAutoWorkspaceTitleInSplitWorkspace waits for a
  panel title to propagate. The .ghosttyDidSetTitle observer is registered with
  queue: .main, so it runs as a queued main-queue operation rather than inline with
  the post, and the apply is deferred again by the panel title coalescer's default
  1/30s delay. A second of Thread.sleep starves both hops. Unlike the other two it
  fails at the behavior the test is named after, with earlier assertions passing.

Renaming the blocking helper to waitForConditionBlocking makes all three resolve to
their file-private helpers again, with the same budgets and poll intervals. Its
three existing callers wait on a socket accumulator filled off the main thread, so
they keep the blocking form and are unaffected. A future waitForCondition(timeout:)
in a file without a pumping helper now fails to compile instead of quietly
blocking.
@ejc3
ejc3 force-pushed the fix/blocking-wait-shadowing branch from c5bd939 to b008c6c Compare July 25, 2026 04:56
@austinywang
austinywang merged commit 2ed7358 into manaflow-ai:main Aug 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants